Skip to content

Automated Test: plugins/rename-instrumentation-middleware-to-metrics-middleware #346

Conversation

admin-coderabbit
Copy link
Owner

@admin-coderabbit admin-coderabbit commented Feb 4, 2026

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added contextual logging for plugin operations, automatically enriching logs with endpoint, plugin ID, and datasource information.
    • Introduced context-based logger creation for improved logging context propagation.
  • Improvements

    • Modernized logging interfaces with enhanced type safety and consistency.
    • Simplified logging middleware to derive contextual information from request context, resulting in cleaner, more informative logs.

…are (#76186)

* Plugins: Chore: Renamed instrumentation middleware to metrics middleware

* Removed repeated logger attributes in middleware and contextual logger

* renamed loggerParams to logParams

* PR review suggestion

* Add contextual logger middleware

* Removed unused params from logRequest

* Removed unwanted changes

* Safer FromContext method

* Removed traceID from logParams
@coderabbit-eval
Copy link

coderabbit-eval bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

The changes add a new FromContext() method to the logging interface and implementations to enable context-based logger creation. Variadic parameters across Logger and PrettyLogger methods are updated from interface{} to any for improved type safety. A new ContextualLoggerMiddleware is introduced to enrich request contexts with plugin-specific attributes. The logger middleware is refactored to use context-derived loggers, and InstrumentationMiddleware is renamed to MetricsMiddleware with contextual logging instrumentation removed.

Changes

Cohort / File(s) Summary
Logger Interface & Implementation Updates
pkg/plugins/log/ifaces.go, pkg/plugins/log/fake.go, pkg/plugins/log/logger.go
Added FromContext(ctx context.Context) Logger method to Logger interface and implementations. Updated all variadic parameters in Logger and PrettyLogger methods from interface{} to any for stronger typing.
New Contextual Logging Middleware
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go
Introduced new ContextualLoggerMiddleware type with factory function and methods for QueryData, CallResource, CheckHealth, CollectMetrics, SubscribeStream, PublishStream, and RunStream. Enriches request context with endpoint, pluginId, dsName, dsUID, and username attributes.
Logger Middleware Refactoring
pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go
Simplified logRequest() signature to extract logger from context via FromContext() instead of accepting explicit pluginCtx and endpoint parameters. Removed tracing imports and context-dependent logging parameters.
Metrics Middleware Rename & Cleanup
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go, pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go
Renamed InstrumentationMiddleware to MetricsMiddleware and updated constructor names. Removed contextual logging instrumentation and log package dependency from metrics middleware.
Middleware Chain Integration
pkg/services/pluginsintegration/pluginsintegration.go
Updated CreateMiddlewares to replace InstrumentationMiddleware with MetricsMiddleware and insert new ContextualLoggerMiddleware into the plugin client middleware chain.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Plugin Client
    participant CLM as ContextualLoggerMiddleware
    participant LM as LoggerMiddleware
    participant PM as MetricsMiddleware
    participant Handler as Underlying Handler

    Client->>CLM: QueryData(ctx, req)
    CLM->>CLM: instrumentContext(ctx)
    CLM->>CLM: Enrich context with<br/>endpoint, pluginId,<br/>dsName, dsUID, uname
    CLM->>LM: QueryData(enriched_ctx, req)
    LM->>LM: logRequest(ctx, fn)
    LM->>LM: logger.FromContext(ctx)
    LM->>PM: QueryData(ctx, req)
    PM->>PM: Record metrics
    PM->>Handler: QueryData(ctx, req)
    Handler-->>PM: response
    PM-->>LM: response
    LM-->>CLM: response
    CLM-->>Client: response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hoppy changes in the logs today!
Context flows in a contextual way,
From interface{} to any we hop,
MiddleWAREs stack without a stop! 🔗
The logger now remembers its place,
Through every request, every trace! ✨

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: renaming InstrumentationMiddleware to MetricsMiddleware across multiple files in the plugins integration module.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch plugins/rename-instrumentation-middleware-to-metrics-middleware

Comment @coderabbit eval help to get the list of available commands and usage tips.

@coderabbit-eval coderabbit-eval bot closed this Feb 4, 2026
Copy link

@coderabbit-eval coderabbit-eval bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/plugins/log/ifaces.go`:
- Around line 6-24: The Logger interface now requires
FromContext(context.Context) Logger; update both logWrapper types named
logWrapper in pkg/plugins/backendplugin/grpcplugin/log_wrapper.go and
pkg/plugins/backendplugin/coreplugin/registry.go to implement FromContext by
delegating to their embedded/field Logger (call the underlying logger's
FromContext and return the resulting Logger), ensuring the method signature
matches Logger.FromContext and returns the contextual wrapper if needed to
preserve wrapper behavior.
🧹 Nitpick comments (5)
pkg/plugins/log/fake.go (1)

46-48: FromContext returns a disconnected logger — log assertions may silently miss entries.

FromContext returns a brand-new TestLogger with its own Logs fields, just like New does. Any code path that calls logger.FromContext(ctx).Info(...) (e.g., LoggerMiddleware.logRequest) will write to a logger that no test holds a reference to, so assertions on the original TestLogger's InfoLogs, ErrorLogs, etc. will never see those entries.

If test coverage for LoggerMiddleware log output matters, consider returning the receiver (f) instead, or at least documenting this behavior.

Possible fix (return receiver)
 func (f *TestLogger) FromContext(_ context.Context) Logger {
-	return NewTestLogger()
+	return f
 }
pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go (1)

21-21: Test function name is stale after the rename.

The type was renamed from InstrumentationMiddleware to MetricsMiddleware, but the test function is still TestInstrumentationMiddleware. Consider renaming it to TestMetricsMiddleware for consistency.

pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go (1)

75-82: Stale variable name imw (instrumentation middleware).

imw is a leftover abbreviation from InstrumentationMiddleware. Consider renaming to mmw or mw for consistency with the MetricsMiddleware type.

Suggested rename
 func NewMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service) plugins.ClientMiddleware {
-	imw := newMetricsMiddleware(promRegisterer, pluginRegistry)
+	mw := newMetricsMiddleware(promRegisterer, pluginRegistry)
 	return plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client {
-		imw.next = next
-		return imw
+		mw.next = next
+		return mw
 	})
 }
pkg/plugins/log/ifaces.go (1)

21-23: Doc comment for FromContext is identical to New and doesn't convey the distinction.

Line 22 says "returns a new contextual Logger that has this logger's context plus the given context" — the same wording as New on line 7. FromContext retrieves logger attributes stored in a context.Context (e.g., by middleware), whereas New accepts explicit key/value pairs. Consider making the doc more specific.

Suggested doc
-	// FromContext returns a new contextual Logger that has this logger's context plus the given context.
+	// FromContext returns a Logger enriched with attributes previously stored in the given context.
 	FromContext(ctx context.Context) Logger
pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go (1)

59-69: Consider instrumenting stream methods for consistency.

SubscribeStream, PublishStream, and RunStream all have access to req.PluginContext but skip instrumentContext. This means downstream middleware/handlers for streaming won't have the contextual logging attributes. If this omission is intentional (e.g., long-lived streams where per-request context is less useful), a brief comment explaining the rationale would help future readers.

Comment on lines 6 to 24
type Logger interface {
// New returns a new contextual Logger that has this logger's context plus the given context.
New(ctx ...interface{}) Logger
New(ctx ...any) Logger

// Debug logs a message with debug level and key/value pairs, if any.
Debug(msg string, ctx ...interface{})
Debug(msg string, ctx ...any)

// Info logs a message with info level and key/value pairs, if any.
Info(msg string, ctx ...interface{})
Info(msg string, ctx ...any)

// Warn logs a message with warning level and key/value pairs, if any.
Warn(msg string, ctx ...interface{})
Warn(msg string, ctx ...any)

// Error logs a message with error level and key/value pairs, if any.
Error(msg string, ctx ...interface{})
Error(msg string, ctx ...any)

// FromContext returns a new contextual Logger that has this logger's context plus the given context.
FromContext(ctx context.Context) Logger
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if Logger interface is used/implemented outside the plugins/log package
rg -n --type=go 'log\.Logger' -g '!pkg/plugins/log/*' -g '!vendor/*' | head -40

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 4598


🏁 Script executed:

# Also search for direct implementations of the Logger interface
# Look for methods like Debug, Info, Warn, Error to find implementations
rg -n --type=go 'func.*Debug.*msg string' -g '!vendor/*' | head -20

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 742


🏁 Script executed:

# Search for any type implementing Logger interface patterns
rg -n --type=go 'type.*struct' -A 5 | grep -A 5 'Debug\|Info\|Warn\|Error' | head -30

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1569


🏁 Script executed:

# Check the file to understand the full context
cat -n pkg/plugins/log/ifaces.go

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1609


🏁 Script executed:

# Check each implementation for FromContext method
echo "=== Checking logWrapper in grpcplugin ===" && \
rg -A 30 'type logWrapper' pkg/plugins/backendplugin/grpcplugin/log_wrapper.go | head -50

echo -e "\n=== Checking logWrapper in coreplugin ===" && \
rg -A 30 'type logWrapper' pkg/plugins/backendplugin/coreplugin/registry.go | head -50

echo -e "\n=== Checking grafanaInfraLogWrapper ===" && \
rg -A 30 'type grafanaInfraLogWrapper' pkg/plugins/log/logger.go | head -50

echo -e "\n=== Checking TestLogger ===" && \
rg -A 30 'type TestLogger' pkg/plugins/log/fake.go | head -50

echo -e "\n=== Checking Fake logger ===" && \
rg -A 30 'type Fake' pkg/infra/log/logtest/fake.go | head -50

echo -e "\n=== Checking ConcreteLogger ===" && \
rg -A 30 'type ConcreteLogger' pkg/infra/log/log.go | head -50

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 3884


🏁 Script executed:

# Check if FromContext method exists in any of these implementation files
echo "=== Searching for FromContext implementations ===" && \
rg -n 'FromContext' pkg/plugins/ pkg/infra/log/

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 1691


🏁 Script executed:

# Check if the implementations in grpcplugin and coreplugin have FromContext
echo "=== Checking grpcplugin/log_wrapper.go for FromContext ===" && \
rg -n 'FromContext' pkg/plugins/backendplugin/grpcplugin/log_wrapper.go

echo -e "\n=== Checking coreplugin/registry.go for FromContext ===" && \
rg -n 'FromContext' pkg/plugins/backendplugin/coreplugin/registry.go

# Get the full implementation of both logWrapper types to see all methods
echo -e "\n=== Full logWrapper in grpcplugin ===" && \
cat -n pkg/plugins/backendplugin/grpcplugin/log_wrapper.go

echo -e "\n=== Full logWrapper in coreplugin ===" && \
rg -B 5 -A 100 'type logWrapper struct' pkg/plugins/backendplugin/coreplugin/registry.go | head -100

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 6227


Update internal Logger interface implementations to include the FromContext method.

Adding FromContext to the Logger interface is a breaking change. Two internal adapter types are missing the implementation and will fail to compile:

  • logWrapper in pkg/plugins/backendplugin/grpcplugin/log_wrapper.go
  • logWrapper in pkg/plugins/backendplugin/coreplugin/registry.go

Both should delegate to their underlying Logger field's FromContext method to satisfy the interface.

🤖 Prompt for AI Agents
In `@pkg/plugins/log/ifaces.go` around lines 6 - 24, The Logger interface now
requires FromContext(context.Context) Logger; update both logWrapper types named
logWrapper in pkg/plugins/backendplugin/grpcplugin/log_wrapper.go and
pkg/plugins/backendplugin/coreplugin/registry.go to implement FromContext by
delegating to their embedded/field Logger (call the underlying logger's
FromContext and return the resulting Logger), ensuring the method signature
matches Logger.FromContext and returns the contextual wrapper if needed to
preserve wrapper behavior.

Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant